20 new working GeneralStateTests #148
Conversation
|
The Travis CI build is failing on both Linux and macOS because: It was mentioned in |
|
LGTM. |
|
At least 3 or 4 of them ( |
|
|
||
| # Don't duplicate zero-padding of mem.extend | ||
| let paddingOffset = memPos + sourceBytes.len | ||
| mem.write(paddingOffset, repeat(paddingValue, max(prevLen - paddingOffset, 0))) |
There was a problem hiding this comment.
Can we avoid the call to repeat here? It performs a memory allocations that's not really necessary.
The previous code is more convoluted for sure, but it actually seems more economical to me when it comes to branching. There is a potential hidden branch in every call to min and max and you would still call mem.write even when repeat returns a zero-len sequence.
There was a problem hiding this comment.
I think min/max are optimised to not need branches in most compilers, being replaced with cmp and conditional mov, and it does make for easier to read code. However I think there's a lot to be said for explicit code when performance matters.
There was a problem hiding this comment.
@zah yes, I'll look into a better approach than repeat. I share your concern here. In this version I went for readability, but to be fair, this code's seems plausibly more performance-sensitive than most in Nimbus. It's worth optimizing.
@coffeepots yes, that min and max are often branchless was part of my thought process. The issue I had with the explicit code was that it created multiple subcases that I found harder to verify were all correct, created some duplication, and obscured the degree to which the different nominally disjoint code flows were similar in practice. I'm not strongly opinionated about this though.
In no particular order:
writePaddedResultto always write padding for non-extended sections of memory up to the total number of bytes requested written, regardless of input data size. The correct semantics are not to leave them alone, as the previouswritePaddedResultdid. I find this version a bit simpler, too -- no branches, for example.LOG1,LOG2,LOG3, andLOG4opcodes to usecleanMemRef, to solve the usual issues that solves. This required movingcleanMemReftonimbus/vm/interpreter/utils/utils_numeric.nim, where it belongs regardless.UInt256)validateGtecommented lines from the auto-conversion.db.increaseBalance(currentCoinbase, gas_cost)to the end of each codepath (yet to be re-merged; e.g., the two error paths are intentionally identical, and most of them will collapse entirely once transactional database usage comes in), separate from its pairdb.setBalance(sender, db.getBalance(sender) - gas_cost). This has created a bit more duplication temporarily, but solved around a dozenGeneralStateTests.Progress:
CALL, orCREATE, and of which 3 (tests/fixtures/GeneralStateTests/stSystemOperationsTest/suicideCoinbase.json,tests/fixtures/GeneralStateTests/stTransactionTest/TransactionFromCoinbaseHittingBlockGasLimit1.json, andtests/fixtures/GeneralStateTests/stTransactionTest/SuicidesMixingCoinbase.json) have neither those nor an emptytransaction.tofield.